Skip to content

Rely on @checked in OverflowContexts and reexport #16

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

BioTurboNick
Copy link

@BioTurboNick BioTurboNick commented May 3, 2024

As discussed in #12, merging some of the functionality of this package with OverflowContexts (https://github.com/JuliaMath/OverflowContexts.jl) is desired, as is using its name. The name is also preferred for that portion, as it would be inclusive of other modes, e.g., saturating math.

OverflowContexts. expanded the @checked macro of this package and expanded it to include other features including @unchecked, allowing these to be arbitrarily nested, and providing the capability to set a default for a module with @default_checked and @default_unchecked at the top of a module. It also fixes some limitations with the implementation here.

This PR removes the portions that are present in OverflowContexts and adds OverflowContexts as a dependency (starting with newly-registered v0.2.4).

All tests of this package passed with the dependency added and the native macro removed before I removed those specific tests here. For compatibility, CheckedArithmetic will re-exports the portions of OverflowContexts necessary for there to be no break for users of CheckedArithmetic.

@kimikage
Copy link
Collaborator

kimikage commented May 5, 2024

I do not see a problem with this change itself.
However, do we need to maintain CheckedArithmetic now?
Since CheckedArithmetic is not obviously broken, there may be no need to add a dependency until a concrete problem arises.
I just want to minimize maintenance costs.

@BioTurboNick
Copy link
Author

BioTurboNick commented May 5, 2024

JuliaHub reports 0 dependents on this or CheckedArithmeticCore, so I guess no real need.

There are a couple things in here that I didn't move over... but I guess they just map values to (U)Int128s for "safer" math? Is that functionality useful to keep around? If not, maybe deprecation is in order.

Perhaps we can add a note about OverflowContexts to the README at minimum.

@kimikage
Copy link
Collaborator

kimikage commented May 5, 2024

Perhaps we can add a note about OverflowContexts to the README at minimum.

Yes, I agree.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants